Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
Greptile SummaryThis PR exposes Confidence Score: 5/5Safe to merge; backward-compatible addition with no logic errors and a matching default. All findings are P2 style suggestions. The new property correctly defaults to 11 pt (identical to the static constant it replaces), is threaded through every relevant view, and all existing presets explicitly or implicitly use the same value. No behavioural change for current callers. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[BonsplitConfiguration.Appearance\ntabTitleFontSize: CGFloat = 11] -->|passed as appearance| B[TabItemView]
A -->|passed as appearance| C[TabDragPreview]
B -->|size: tabTitleFontSize| D[Tab Title Text]
B -->|size: max8 tabTitleFontSize-2| E[Zoom Indicator Icon]
B -->|size: max8 tabTitleFontSize-2| F[Shortcut Hint Text]
B -->|size: max8 tabTitleFontSize-2| G[shortcutHintWidth NSFont]
C -->|size: tabTitleFontSize| H[Drag Preview Title Text]
Reviews (1): Last reviewed commit: "feat: configurable tab title font size" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| Text(tab.title) | ||
| .font(.system(size: TabBarMetrics.titleFontSize)) | ||
| .font(.system(size: appearance.tabTitleFontSize)) |
There was a problem hiding this comment.
The zoom indicator and shortcut hint already protect against a sub-8 pt size via max(8, appearance.tabTitleFontSize - 2), but the main tab title text uses appearance.tabTitleFontSize directly. A caller who passes 0 or a very small value will silently render invisible tab titles while the secondary elements are clamped. Applying the same floor keeps behaviour consistent across all three callers.
| .font(.system(size: appearance.tabTitleFontSize)) | |
| .font(.system(size: max(8, appearance.tabTitleFontSize))) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/TabItemView.swift (1)
136-137: Extract a shared derived font-size helper for accessory text/icons.
max(8, appearance.tabTitleFontSize - 2)is repeated in three places; centralizing it avoids drift and keeps sizing logic easier to maintain.♻️ Suggested refactor
struct TabItemView: View { @@ + private var accessoryFontSize: CGFloat { + max(8, appearance.tabTitleFontSize - 2) + } @@ Image(systemName: "arrow.up.left.and.arrow.down.right") - .font(.system(size: max(8, appearance.tabTitleFontSize - 2), weight: .semibold)) + .font(.system(size: accessoryFontSize, weight: .semibold)) @@ - let font = NSFont.systemFont(ofSize: max(8, appearance.tabTitleFontSize - 2), weight: .semibold) + let font = NSFont.systemFont(ofSize: accessoryFontSize, weight: .semibold) @@ Text(shortcutHintLabel) - .font(.system(size: max(8, appearance.tabTitleFontSize - 2), weight: .semibold, design: .rounded)) + .font(.system(size: accessoryFontSize, weight: .semibold, design: .rounded))Also applies to: 227-229, 237-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/TabItemView.swift` around lines 136 - 137, Create a single helper to compute the accessory font size (e.g., a private computed property or method named accessoryFontSize or derivedAccessoryFontSize in the TabItemView struct) that returns max(8, appearance.tabTitleFontSize - 2), then replace the three inline occurrences of max(8, appearance.tabTitleFontSize - 2) (used in the TabItemView body for accessory text/icons and the font modifiers at the spots around the current .font(.system(size: ...)) calls) with a call to that helper; keep the helper private to TabItemView (or as an Appearance extension if used elsewhere) and use it in all places referenced in this file so the sizing logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabItemView.swift`:
- Around line 136-137: Create a single helper to compute the accessory font size
(e.g., a private computed property or method named accessoryFontSize or
derivedAccessoryFontSize in the TabItemView struct) that returns max(8,
appearance.tabTitleFontSize - 2), then replace the three inline occurrences of
max(8, appearance.tabTitleFontSize - 2) (used in the TabItemView body for
accessory text/icons and the font modifiers at the spots around the current
.font(.system(size: ...)) calls) with a call to that helper; keep the helper
private to TabItemView (or as an Appearance extension if used elsewhere) and use
it in all places referenced in this file so the sizing logic is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a0bf21e-fac7-4e70-a4a5-fb66e0b7e3f0
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/TabDragPreview.swiftSources/Bonsplit/Internal/Views/TabItemView.swiftSources/Bonsplit/Public/BonsplitConfiguration.swift
1 participant
Adds a public appearance-level
tabTitleFontSizesetting to Bonsplit so host apps can control horizontal tab label sizing without patching internal metrics.Changes:
BonsplitConfiguration.Appearance.tabTitleFontSizewith a default of11.appearance.tabTitleFontSizefor tab labels inTabItemView.TabDragPreviewso drag visuals stay consistent with the tab strip.This keeps the change host-configurable while preserving the existing default look.
Summary by cubic
Make tab title font size configurable via a new appearance setting so host apps can control label sizing. Applies to the tab bar, drag preview, and scales accessory icons and hit areas to keep them readable.
BonsplitConfiguration.Appearance.tabTitleFontSize(default: 11).TabItemViewandTabDragPreviewuse this value.tabTitleFontSize.Written for commit d1576ed. Summary will update on new commits.
Summary by CodeRabbit